Skip to content

feat(0.2): stable finding IDs (Track 4.4)#138

Open
pmclSF wants to merge 1 commit intomainfrom
feat/0.2-finding-ids
Open

feat(0.2): stable finding IDs (Track 4.4)#138
pmclSF wants to merge 1 commit intomainfrom
feat/0.2-finding-ids

Conversation

@pmclSF
Copy link
Copy Markdown
Owner

@pmclSF pmclSF commented May 2, 2026

Summary

Track 4.4 from the 0.2.0 parity-gated release plan. Foundational deliverable that unblocks three downstream features:

  • Track 4.5 — `.terrain/suppressions.yaml` (suppressions match against FindingID)
  • Track 4.6 — `terrain explain finding ` round-trip
  • Track 4.8 — `--new-findings-only --baseline `

Format

```
{detector}@{normalized_path}:{anchor}#{hash}
```

Example: `weakAssertion@internal/auth/login_test.go:TestLogin#a1b2c3d4`

Components:

  • `detector` — signal type
  • `normalized_path` — forward-slash, repo-relative
  • `anchor` — symbol name when present; `L` otherwise; `_` when neither
  • `hash` — 8 hex chars from the canonical form for collision resistance

Human-readable enough to mention in PR comments; unique enough that two findings of the same type on the same line (different symbols) get distinct IDs.

Stability guarantees

  • Same (Type, file, symbol, line) → same ID across runs.
  • Symbol takes precedence as anchor → line drift within a symbol does NOT change the ID. This covers the common case (whitespace edits, import reordering).
  • File rename or symbol rename → new ID. The finding has moved.
  • Line drift WITHOUT a symbol → new ID. Known limitation; AST-anchored 0.3 work removes it.

What landed

  • `internal/identity/finding_id.go` (~145 LOC) — `BuildFindingID` / `ParseFindingID` / `MatchFindingID` + helpers. Reuses `GenerateID` + `NormalizePath` from the existing identity package.
  • `internal/identity/finding_id_test.go` (~200 LOC) — 16 tests: stability / shape / rename / file-move / detector-change / path-normalization (back- vs forward-slash) / line-anchor / placeholder / symbol-precedence / line-drift / parser round-trip / malformed rejection / anchor-with-colons / MatchFindingID semantics.
  • `internal/models/signal.go` — new `FindingID` field, `omitempty` for back-compat.
  • `internal/engine/finding_ids.go` — `assignFindingIDs(snapshot)` walks top-level + per-file signals, populates `FindingID` for any signal that doesn't have one. Idempotent. Pre-set IDs preserved (for specialized detectors).
  • `internal/engine/pipeline.go` — call `assignFindingIDs` right after `SortSnapshot` in Step 10. Sort runs first → IDs land in canonical order → byte-identical `SOURCE_DATE_EPOCH` output preserved.
  • `internal/engine/finding_ids_test.go` — 4 tests: top-level + per-file population, pre-set preserved, idempotency, nil-safe.

Why models doesn't import identity

`internal/models/` has zero internal imports today. Keeping it dependency-free is valuable for serialization. The engine layer is the natural place for ID assignment — engine → identity matches the layering elsewhere.

Test plan

  • `go test ./internal/identity/` green (16 new tests)
  • `go test ./internal/engine/` green (4 new tests)
  • `go test ./...` green
  • `go test ./internal/testdata/` green (goldens unchanged — FindingID is `omitempty` and existing goldens don't assert presence; 0.2.1 work updates goldens to include IDs)

Plan link

`/Users/pzachary/.claude/plans/kind-mapping-turing.md` (Track 4.4).

🤖 Generated with Claude Code

Foundational deliverable for the Gate-pillar parity lift. Stable IDs
unblock three downstream features: suppressions
(`.terrain/suppressions.yaml` honoring per-finding entries),
`terrain explain finding <id>` round-trip, and
`--new-findings-only --baseline <path>` baseline-aware gating.

Format:
  {detector}@{normalized_path}:{anchor}#{hash}

Example: weakAssertion@internal/auth/login_test.go:TestLogin#a1b2c3d4

Where:
  detector       = signal type (e.g. "weakAssertion")
  normalized_path = forward-slash, repo-relative
  anchor         = symbol when present, "L<line>" otherwise, "_" when neither
  hash           = 8 hex chars derived from canonical form

Stability guarantees (documented in the FindingID field doc-comment
and the package-level comment in `internal/identity/finding_id.go`):

  * Same (Type, Location.File, Location.Symbol, Location.Line) →
    same FindingID across runs. Used for suppression matching and
    baseline-aware gating.
  * Symbol takes precedence as the anchor when present, so line
    drift WITHIN a symbol does not change the ID. This is the
    common case (whitespace edits, import reordering).
  * File rename or symbol rename produces a new ID. The underlying
    finding has moved; old suppressions don't apply.
  * Line drift WITHOUT a symbol changes the ID — known limitation
    documented; AST-anchored 0.3 work removes it.

What landed:

  internal/identity/finding_id.go (new, ~145 lines)
    BuildFindingID / ParseFindingID / MatchFindingID + helpers.
    Reuses GenerateID + NormalizePath from the existing identity
    package.

  internal/identity/finding_id_test.go (new, ~200 lines)
    16 table-driven tests covering: stability, shape, distinct on
    rename/file-move/detector-change, path normalization (back- vs
    forward-slash), line anchor when no symbol, placeholder when
    nothing, symbol-precedence-over-line invariant, line drift
    changes ID without symbol, parser round-trip + malformed
    rejection (8 cases), anchor-with-colons round-trip, MatchFindingID
    semantics.

  internal/models/signal.go
    New `FindingID string `json:"findingId,omitempty"` field on
    models.Signal with the stability documentation inline. omitempty
    so older snapshots remain valid; pre-existing JSON consumers
    that don't read the field are unaffected.

  internal/engine/finding_ids.go (new)
    assignFindingIDs(snapshot) — walks top-level Signals + per-file
    Signals, populates FindingID for any signal that doesn't already
    have one. Pre-set IDs are preserved (lets specialized detectors
    like detectorPanic emit their own anchor). Idempotent and
    nil-safe.

  internal/engine/pipeline.go
    Calls assignFindingIDs(snapshot) right after SortSnapshot in
    Step 10 of RunPipelineContext. Sort runs first so IDs land in
    canonical order; this preserves byte-identical SOURCE_DATE_EPOCH
    output.

  internal/engine/finding_ids_test.go (new)
    4 tests: top-level + per-file population, pre-set ID preserved,
    idempotency, nil-safe.

Why the model package isn't importing identity directly: keeps
`internal/models/` dependency-free (zero internal imports today).
Engine orchestrates detectors and is the natural place for
ID assignment; the dependency arrow runs engine → identity, which
matches the layering elsewhere.

Verification:
  go test ./internal/identity/ ./internal/engine/ — green
  go test ./... — green
  go test ./internal/testdata/ — green (goldens unchanged because
    FindingID is omitempty and the existing goldens don't assert
    its presence; 0.2.1 work updates goldens to include IDs)

Next:
  Track 4.5 — `.terrain/suppressions.yaml` minimal viable shape;
    suppressions match against FindingID
  Track 4.6 — `terrain explain finding <id>` lookup
  Track 4.7 — `terrain suppress <id>` writer
  Track 4.8 — `--new-findings-only --baseline <path>`

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

Terrain AI Risk Review

Metric Value
AI surfaces 13
Eval scenarios 16
Impacted scenarios 0
Uncovered surfaces 13

Decision: PASS — AI surfaces are covered.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

[RISK] Terrain — Merge with caution

High-severity gaps found in changed code.

Metric Value
Changed files 6 (4 source · 2 test)
Impacted units 23
Protection gaps 3
Tests selected 57 of 774 (7% of suite)

Coverage gaps in changed code

  • internal/engine/finding_ids.go [LOW] — finding_ids.go has no observed test coverage.
    → Add unit tests for finding_ids.go.
  • internal/engine/pipeline.go [MED] — Exported function ProgressFunc has no observed test coverage.
    → Add unit tests for exported function ProgressFunc — this is public API surface.
  • internal/engine/pipeline.go [MED] — Exported function RunPipelineContext has no observed test coverage.
    → Add unit tests for exported function RunPipelineContext — this is public API surface.
4 pre-existing issues on changed files
  • internal/engine/finding_ids.go [HIGH] — [blastRadiusHotspot] Changes to this file propagate to 1168 tests (148 direct, 1020 indirect). High blast radius increases regression risk.
  • internal/engine/pipeline.go [HIGH] — [blastRadiusHotspot] Changes to this file propagate to 1168 tests (148 direct, 1020 indirect). High blast radius increases regression risk.
  • internal/identity/finding_id.go [HIGH] — [blastRadiusHotspot] Changes to this file propagate to 215 tests (41 direct, 174 indirect). High blast radius increases regression risk.
  • internal/models/signal.go [HIGH] — [blastRadiusHotspot] Changes to this file propagate to 2154 tests (1507 direct, 647 indirect). High blast radius increases regression risk.

Recommended tests

57 test(s) with exact coverage of 20 impacted unit(s). 3 impacted unit(s) have no covering tests in the selected set.

Package Tests Sample
internal/engine 8 internal/engine/artifacts_test.go ...
internal/reporting 5 internal/reporting/analyze_report_test.go ...
cmd/terrain 4 cmd/terrain/ai_workflow_test.go ...
internal/models 4 internal/models/signal_v2_test.go ...
internal/signals 3 internal/signals/detector_registry_test.go ...
internal/testdata 3 internal/testdata/adversarial_test.go ...
internal/aidetect 2 internal/aidetect/embedding_model_change_test.go ...
internal/analyze 2 internal/analyze/analyze_golden_test.go ...
internal/insights 2 internal/insights/insights_golden_test.go ...
internal/ownership 2 internal/ownership/aggregate_test.go ...
internal/quality 2 internal/quality/snapshot_heavy_test.go ...
internal/benchmark 1 internal/benchmark/export_test.go
internal/calibration 1 internal/calibration/runner_test.go
internal/changescope 1 internal/changescope/changescope_test.go
internal/comparison 1 internal/comparison/compare_test.go
internal/explain 1 internal/explain/explain_test.go
internal/gauntlet 1 internal/gauntlet/ingest_test.go
internal/governance 1 internal/governance/evaluate_test.go
internal/graph 1 internal/graph/graph_test.go
internal/heatmap 1 internal/heatmap/heatmap_test.go
internal/identity 1 internal/identity/finding_id_test.go
internal/measurement 1 internal/measurement/measurement_test.go
internal/metrics 1 internal/metrics/metrics_test.go
internal/migration 1 internal/migration/readiness_test.go
internal/scoring 1 internal/scoring/risk_engine_test.go
internal/server 1 internal/server/server_test.go
internal/severity 1 internal/severity/rubric_test.go
internal/skipstats 1 internal/skipstats/summary_test.go
internal/stability 1 internal/stability/cluster_test.go
internal/structural 1 internal/structural/structural_test.go
internal/summary 1 internal/summary/executive_test.go

Owners: PMCLSF

Limitations
  • No coverage artifacts provided; protection gaps reflect missing data, not measured absence. Provide --coverage to improve accuracy.
  • Mixed test cultures reduce cross-framework optimization confidence. Consider standardizing on fewer frameworks.

Generated by Terrain · terrain pr --json for machine-readable output

Targeted Test Results

Terrain selected 57 test(s) instead of the full suite.

  • Go tests: passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant